Skip to content

Error displayed while fetching rollout revisions details (#10132)#255

Merged
keithchong merged 1 commit into
redhat-developer:mainfrom
keithchong:10132-LoadingIsNotError
Jul 8, 2026
Merged

Error displayed while fetching rollout revisions details (#10132)#255
keithchong merged 1 commit into
redhat-developer:mainfrom
keithchong:10132-LoadingIsNotError

Conversation

@keithchong

@keithchong keithchong commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

See GITOPS-10132 for details. I've pulled out the Loading into its own component

@openshift-ci openshift-ci Bot requested a review from wtam2018 July 7, 2026 12:24
@keithchong keithchong requested review from aali309 and removed request for wtam2018 July 7, 2026 12:24
@codecov-commenter

codecov-commenter commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 11.83%. Comparing base (01e9db1) to head (2c0d401).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
...gitops/utils/components/LoadingSpinner/Loading.tsx 0.00% 7 Missing ⚠️
.../gitops/components/rollout/RolloutRevisionsTab.tsx 0.00% 4 Missing ⚠️
src/gitops/topology/console/pod-traffic.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #255      +/-   ##
==========================================
- Coverage   11.92%   11.83%   -0.10%     
==========================================
  Files         154      155       +1     
  Lines        6272     6330      +58     
  Branches     2028     2110      +82     
==========================================
+ Hits          748      749       +1     
- Misses       5524     5581      +57     
Flag Coverage Δ
unit-tests 11.83% <0.00%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@keithchong, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 21 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: dcf5684f-0dd3-4080-a722-d04d03ad4f87

📥 Commits

Reviewing files that changed from the base of the PR and between cacdace and 2c0d401.

📒 Files selected for processing (3)
  • src/gitops/components/rollout/RolloutRevisionsTab.tsx
  • src/gitops/topology/console/pod-traffic.tsx
  • src/gitops/utils/components/LoadingSpinner/Loading.tsx
📝 Walkthrough

Walkthrough

A new shared Loading spinner component module was added, exporting Loading and LoadingInline. RolloutRevisionsTab was refactored to separate error and loading conditions, using the new Loading component. pod-traffic.tsx now imports LoadingInline from the shared module instead of defining it locally.

Changes

Shared Loading component adoption

Layer / File(s) Summary
New shared Loading component
src/gitops/utils/components/LoadingSpinner/Loading.tsx
Adds Loading and LoadingInline exported components wrapping a PatternFly Spinner with accessibility attributes and optional inline styling.
RolloutRevisionsTab loading/error split
src/gitops/components/rollout/RolloutRevisionsTab.tsx
Imports the new Loading component and refactors the render logic into separate error and loading branches before rendering Revisions.
pod-traffic loading component reuse
src/gitops/topology/console/pod-traffic.tsx
Replaces local LoadingInline, Loading, and LoadingProps definitions with an import of LoadingInline from the shared module.

Estimated code review effort: 1 (Trivial) | ~5 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The description is related via the referenced issue, but it is too vague to describe the change itself. Add a brief summary of the fix or the affected rollout revisions loading behavior instead of only linking the issue.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: fixing the rollout revisions error shown during loading.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
src/gitops/utils/components/LoadingSpinner/Loading.tsx (1)

4-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reorder declarations for readability.

LoadingInline (Line 4) references Loading before it is declared (Line 7). It works because the reference is inside a deferred function body, but declaring Loading first improves readability.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/gitops/utils/components/LoadingSpinner/Loading.tsx` around lines 4 - 14,
Reorder the component declarations in Loading.tsx so Loading is defined before
LoadingInline for readability. Keep the behavior the same by leaving
LoadingInline as a thin wrapper around Loading and preserving
LoadingInline.displayName, but move the Loading function declaration above
LoadingInline to make the dependency order explicit.
src/gitops/components/rollout/RolloutRevisionsTab.tsx (1)

67-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Nested ternary flagged by ESLint.

Consider refactoring to an early-return/if-else style for readability, per the static analysis hint (no-nested-ternary).

♻️ Suggested refactor
-      {loadError || podsloadError ? (
-        <div>
-          ...
-        </div>
-      ) : !loaded || !podsloaded ? (
-        <Loading />
-      ) : (
-        <Revisions
-          rollout={rollout}
-          replicaSets={resourceAsArray(replicaSets) as ReplicaSetKind[]}
-          pods={resourceAsArray(pods) as PodKind[]}
-        />
-      )}
+      {(() => {
+        if (loadError || podsloadError) {
+          return (
+            <div>
+              ...
+            </div>
+          );
+        }
+        if (!loaded || !podsloaded) {
+          return <Loading />;
+        }
+        return (
+          <Revisions
+            rollout={rollout}
+            replicaSets={resourceAsArray(replicaSets) as ReplicaSetKind[]}
+            pods={resourceAsArray(pods) as PodKind[]}
+          />
+        );
+      })()}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/gitops/components/rollout/RolloutRevisionsTab.tsx` around lines 67 - 68,
The conditional rendering in RolloutRevisionsTab is using a nested ternary that
triggers the no-nested-ternary lint rule. Refactor the JSX in
RolloutRevisionsTab to use early returns or a simple if/else sequence instead of
chaining ternary branches, keeping the same loading/empty/main render behavior
while making the render logic easier to read and maintain.

Source: Linters/SAST tools

src/gitops/topology/console/pod-traffic.tsx (1)

27-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove dead commented-out code instead of leaving it in-place.

The old LoadingInline/Loading/LoadingProps implementations are commented out rather than deleted now that they've been moved to the shared module. Leaving this dead code adds clutter.

🧹 Suggested cleanup
-// export const LoadingInline: React.FunctionComponent = () => <Loading isInline />;
-// LoadingInline.displayName = 'LoadingInline';
-
-// export const Loading: React.FunctionComponent<LoadingProps> = ({ className, isInline }) => (
-//   <div
-//     className={classNames('co-m-loader', { 'co-m-loader--inline': isInline }, className)}
-//     data-test="loading-indicator"
-//   >
-//     <Spinner aria-live="polite" aria-busy="true" isInline={isInline} size="lg" />
-//   </div>
-// );
-
-// Loading.displayName = 'Loading';
-
-// type LoadingProps = {
-//   className?: string;
-//   isInline?: boolean;
-// };
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/gitops/topology/console/pod-traffic.tsx` around lines 27 - 44, Remove the
commented-out LoadingInline, Loading, and LoadingProps block from
pod-traffic.tsx since those implementations now live in the shared module. Clean
up the dead code in this area and keep the file focused on the active pod
traffic logic and any remaining references to LoadingInline/Loading that are
still in use.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/gitops/components/rollout/RolloutRevisionsTab.tsx`:
- Around line 67-68: The conditional rendering in RolloutRevisionsTab is using a
nested ternary that triggers the no-nested-ternary lint rule. Refactor the JSX
in RolloutRevisionsTab to use early returns or a simple if/else sequence instead
of chaining ternary branches, keeping the same loading/empty/main render
behavior while making the render logic easier to read and maintain.

In `@src/gitops/topology/console/pod-traffic.tsx`:
- Around line 27-44: Remove the commented-out LoadingInline, Loading, and
LoadingProps block from pod-traffic.tsx since those implementations now live in
the shared module. Clean up the dead code in this area and keep the file focused
on the active pod traffic logic and any remaining references to
LoadingInline/Loading that are still in use.

In `@src/gitops/utils/components/LoadingSpinner/Loading.tsx`:
- Around line 4-14: Reorder the component declarations in Loading.tsx so Loading
is defined before LoadingInline for readability. Keep the behavior the same by
leaving LoadingInline as a thin wrapper around Loading and preserving
LoadingInline.displayName, but move the Loading function declaration above
LoadingInline to make the dependency order explicit.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 84746ebe-8c19-4071-adb8-c25b235c871e

📥 Commits

Reviewing files that changed from the base of the PR and between 9086cb7 and cacdace.

📒 Files selected for processing (3)
  • src/gitops/components/rollout/RolloutRevisionsTab.tsx
  • src/gitops/topology/console/pod-traffic.tsx
  • src/gitops/utils/components/LoadingSpinner/Loading.tsx

Signed-off-by: Keith Chong <kykchong@redhat.com>
@keithchong keithchong force-pushed the 10132-LoadingIsNotError branch from cacdace to 2c0d401 Compare July 7, 2026 13:01

@aali309 aali309 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @keithchong

@keithchong keithchong merged commit 4a79605 into redhat-developer:main Jul 8, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants